Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove call to Clip::changeLength #7570

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Remove call to Clip::changeLength #7570

wants to merge 1 commit into from

Conversation

michaelgregorius
Copy link
Contributor

Remove the call to Clip::changeLength from TrackContentWidget::changePosition as it results in bad performance, especially when the Song Editor is in auto-scroll mode and the position is changed very often.

The call in question has set the clip to the length that it already has which in turn resulted in lots of needless calls to Song::updateLength.

It also should not be necessary to update a clip length if all that we do is change the position of the TrackContentWidget.

@michaelgregorius
Remove call to Clip::changeLength
5626a51 
Remove the call to `Clip::changeLength` from
`TrackContentWidget::changePosition` as it results in bad performance,
especially when the Song Editor is in auto-scroll mode and the position
is changed very often.

The call in question has set the clip to the length that it already has
which in turn resulted in lots of needless calls to `Song::updateLength`.

It also should not be necessary to update a clip length if all that we
do is change the position of the `TrackContentWidget`.
@michaelgregorius michaelgregorius mentioned this pull request Nov 3, 2024
Open
1 task
@regulus79
Copy link
Contributor

Do we have any idea as to why Clip::changeLength was put there in the first place? I agree that simply changing the position should not change the length, but are we missing something that the original author of this function was thinking when they wrote this?

@michaelgregorius
Copy link
Contributor Author

michaelgregorius commented Nov 3, 2024 •
edited
Loading

@regulus79, it's not really possible anymore. The line is already there since 19 years. Following the blame brings this line (17 years old):

https://github.com/LMMS/lmms/blame/ddf69feebc2ee6754b897f61ae3bc9101e0df45c/src/core/Track.cpp#L1668

Which is preceded by this line (19 years old):

https://github.com/LMMS/lmms/blame/40017887e9afb4e369522923bcd5c549742353e9/src/core/track.cpp#L1317

Which in turn is preceded by this line (19 years old):

https://github.com/LMMS/lmms/blame/f700c16c339a348664c04e16b56629e588880f0e/src/core/track.cpp#L812

All these commit have originally been made in the SVN repository and have been imported into git. So they are really old. 😅

So, I'd propose to remove the line and if problems should show up to fix them going forward with better code than the removed line. See this comment why it's bad style: #7462 (comment)

Edit: Added the proposal.

@khoidauminh
Copy link
Contributor

I tested the pull request and this is what happens when you zoom using the slider. The the out of bounds clips seem to glitch into view and I can interact with them.

Screencast.From.2024-11-05.14-07-59.mp4
Screencast.From.2024-11-05.14-22-44.mp4

The glitch is not visible when zooming using the mouse wheel, however.

@michaelgregorius
Copy link
Contributor Author

Good catch, @khoidauminh! I can reproduce the issue.

I assume that it is somehow caused by this code in TrackContentWidget::changePosition which adjust the positions of the clips in the track:

lmms/src/gui/tracks/TrackContentWidget.cpp

Lines 269 to 291 in 07baf9e

for (const auto& clipView : m_clipViews)
{
Clip* clip = clipView->getClip();
clip->changeLength( clip->length() );
const int ts = clip->startPosition();
const int te = clip->endPosition()-3;
if( ( ts >= begin && ts <= end ) ||
( te >= begin && te <= end ) ||
( ts <= begin && te >= end ) )
{
clipView->move(static_cast<int>((ts - begin) * ppb / TimePos::ticksPerBar()), clipView->y());
if (!clipView->isVisible())
{
clipView->show();
}
}
else
{
clipView->move(-clipView->width() - 10, clipView->y());
}
}

Line 273 is removed with this PR and it seems to have some surprising side effects on the clip start and end positions which affect the code that follows then.

@JohannesLorenz JohannesLorenz added the rework required Pull requests which must be reworked to make it able to merge or review label Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
rework required Pull requests which must be reworked to make it able to merge or review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@michaelgregorius @regulus79 @khoidauminh @JohannesLorenz